Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify and simplify README #38

Merged
merged 5 commits into from
Sep 28, 2023
Merged

Conversation

asmacdo
Copy link
Member

@asmacdo asmacdo commented Sep 22, 2023

@TheChymera I was pretty liberal in my slashing so lmk if you want to keep any of this.

@yarikoptic
Copy link
Member

conflicts need to be resolved. @TheChymera please review it soon so it could be merged to avoid conflicts to keep coming up

@asmacdo
Copy link
Member Author

asmacdo commented Sep 25, 2023

should have this marked as WIP, but is now ready to review. Following these steps I got a full reanalysis and meta-article generation this morning.

@TheChymera
Copy link
Collaborator

@asmacdo there are a bunch of comments by me above @yarikoptic 's comment. Are they not visible to you guys?

@asmacdo
Copy link
Member Author

asmacdo commented Sep 26, 2023

@TheChymera dont see them, did you start a "review"?

@yarikoptic
Copy link
Member

did you submit the started review?

@@ -19,8 +19,8 @@ ifeq ($(SCRATCH_PATH),)
SCRATCH_PATH = $(PWD)/scratch
endif

OCI_BINARY=docker
SING_BINARY=singularity
OCI_BINARY?=docker
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do differently? = here won't overwrite the variable if you set it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, for me

THE_VAR=foo
.PHONY:
test-env-var:
       echo ${THE_VAR}

THE_VAR is foo, even if I export THE_VAR=somethingelse

When I added the ?= it started working.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange, ok, let's leave it like that then.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@TheChymera
Copy link
Collaborator

Ah, ok, they were saved but I needed to press another button to publish them. These are from yesterday morning, I think one is outdated, let me read again.

README.md Outdated Show resolved Hide resolved
README.md Outdated
- python3-yaml

You will also need to install sourceserifpro font using the tlmgr.
TODO(chymera): audit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audit what?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the list of requirements, since you installed locally

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmacdo ok, can you deal with the rest and I'll figure that one out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceserifpro doesnt appear to be required to finish building anymore. I added this so it would finish building, but this font is listed explicitly as a requirement in rescience.cls

I don't mind if youd like to remove it but lets do that in a separate pr.

- python3-statsmodels
- python3-yaml

You will also need to install sourceserifpro font using the tlmgr.
Copy link
Collaborator

@TheChymera TheChymera Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmacdo

I have tried make article-clean && make article-container without the tlmgr code ( dfced35 ) and it worked. Can you confirm that it also works for you without that? if so, we can drop this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see other

Previous version mistakenly assumed that output pdfs do not need to be
retrieved via datalad if the pdf is regenerated. Even if an OPFVTA
article pdf is generated, we still must retrieve the earlier versons in
order to execute the comparison.
@TheChymera TheChymera merged commit 783504d into con:master Sep 28, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants